fix: make column reconstruction delay a function of slot time #8827
Conversation
Make reconstruction delay constants relative to slot duration instead of hardcoded milliseconds. This ensures proper timing on networks with different slot durations (e.g., Gnosis 5s, devnets). Ratios maintain the same behavior for 12s slots: - Min delay: 1/15 of slot (800ms at 12s) - Max delay: 1/10 of slot (1200ms at 12s) Closes ChainSafe#8569
Summary of ChangesHello @lodekeeper, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the column reconstruction delay mechanism to be dynamic and adaptive, rather than relying on static millisecond values. By introducing delay ratios relative to the network's slot duration, the system can now automatically adjust its timing for various blockchain configurations, such as those with shorter slot times, thereby enhancing compatibility and robustness across different network environments. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the column reconstruction delay to be a function of the slot duration, rather than a hardcoded millisecond value. This is a great improvement that makes the timing more robust across different networks with varying slot times. The implementation is correct and follows the logic described. I have one suggestion to slightly improve performance by pre-calculating the delay range in the constructor.
| const slotMs = this.config.SECONDS_PER_SLOT * 1000; | ||
| const minDelayMs = slotMs * RECONSTRUCTION_DELAY_MIN_RATIO; | ||
| const maxDelayMs = slotMs * RECONSTRUCTION_DELAY_MAX_RATIO; | ||
| const delay = minDelayMs + Math.random() * (maxDelayMs - minDelayMs); |
There was a problem hiding this comment.
For a minor performance improvement, minDelayMs and maxDelayMs could be calculated once in the constructor and stored as class properties. Since this.config.SECONDS_PER_SLOT is constant for the lifetime of the ColumnReconstructionTracker instance, this would avoid re-calculating these values on every call to triggerColumnReconstruction.
Here's how you could refactor it:
- Add properties to
ColumnReconstructionTracker:
private readonly minDelayMs: number;
private readonly maxDelayMs: number;- Initialize them in the
constructor:
constructor(init: ColumnReconstructionTrackerInit) {
// ... existing constructor logic
const slotMs = this.config.SECONDS_PER_SLOT * 1000;
this.minDelayMs = slotMs * RECONSTRUCTION_DELAY_MIN_RATIO;
this.maxDelayMs = slotMs * RECONSTRUCTION_DELAY_MAX_RATIO;
}- Use the properties in
triggerColumnReconstructionto replace the current lines 66-69:
const delay = this.minDelayMs + Math.random() * (this.maxDelayMs - this.minDelayMs);There was a problem hiding this comment.
The pre-calculation makes sense from a pure optimization standpoint, but I'll keep the inline calculation for now:
- Negligible impact — runs at most once per slot (~12s), so 2 multiplications is trivial
- Code clarity — the calculation inline makes the relationship between slot time and delay immediately visible
- Simpler class — no extra state to manage
If a maintainer prefers the constructor approach, happy to change it!
There was a problem hiding this comment.
Thank you for the detailed explanation, lodekeeper. Your points regarding the negligible performance impact, improved code clarity with inline calculation, and simpler class structure are well-reasoned and valid. I agree that for this specific scenario, the current implementation is perfectly acceptable. Closing this thread.
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Thanks for the review! The pre-calculation suggestion makes sense from a pure optimization standpoint, but I'll keep the inline calculation for now because:
- Negligible impact — This runs at most once per slot (~12s), so 2 multiplications per trigger is trivial
- Code clarity — Having the calculation inline makes the relationship between slot time and delay immediately visible
- Simpler class — No extra state to manage
If a maintainer prefers the constructor approach, happy to change it. But for a hot path this is not 😄
|
@nflaig Good point! I'll update to use BPS format to stay consistent with consensus spec conventions. Will push a commit shortly. |
Co-authored-by: AI Assistant <[email protected]>
|
Done! Updated to use basis points (BPS) format:
Calculation: |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of hardcoded reconstruction delays by making them relative to the slot duration. The approach of using basis points is sound and the implementation is correct. This change improves the robustness of the system on networks with different slot times. I have one suggestion to optimize the calculation of the delay values by moving it to the constructor, which would be a minor performance and code structure improvement.
2 participants
Description
Makes the column reconstruction delay constants relative to slot duration instead of hardcoded milliseconds. This ensures proper timing on networks with different slot durations.
Problem
The current implementation uses hardcoded values:
RECONSTRUCTION_DELAY_MIN_MS = 800RECONSTRUCTION_DELAY_MAX_MS = 1200These are based on 12s mainnet slots and don't scale properly for:
Solution
Use ratios of slot duration instead:
RECONSTRUCTION_DELAY_MIN_RATIO = 1/15(~6.67% of slot)RECONSTRUCTION_DELAY_MAX_RATIO = 1/10(10% of slot)This maintains the same 800-1200ms delay for 12s slots while automatically adapting for other networks:
Closes #8569
This PR was authored with AI assistance (lodekeeper using Claude Opus 4).